-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix add_default_workflow #3446
base: dev
Are you sure you want to change the base?
fix add_default_workflow #3446
Conversation
FWIW, this was tested in qiita via a terminal and it works as expected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ty for your patience! I have one concern.
@@ -989,7 +999,7 @@ def _get_predecessors(workflow, node): | |||
init_artifacts = { | |||
wkartifact_type: f'{starting_job.id}:'} | |||
else: | |||
init_artifacts = {wkartifact_type: self.artifact.id} | |||
init_artifacts = {wkartifact_type: str(self.artifact.id)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hate to say it but having to cast an internal id to string is a little bit concerning to me. Internal code should use the numeric form of the artifact_id and it should also be easy to pass the numeric version for any REST call. May I ask why this is needed? I feel like even if it's needed, it would be much clearer to cast it right before it's used to make it obvious why it's needed. Sorry if that's not clear.
@@ -871,7 +881,7 @@ def _get_predecessors(workflow, node): | |||
'artifact transformation'] | |||
merging_schemes = { | |||
qdb.archive.Archive.get_merging_scheme_from_job(j): { | |||
x: y.id for x, y in j.outputs.items()} | |||
x: str(y.id) for x, y in j.outputs.items()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conversion to string for y.id is also not obvious here. Perhaps a comment as to why it's necessary would be appropriate?
No description provided.